Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenEphysBinaryRawIO: Fixing ttl multichan #1603

Merged
merged 6 commits into from
Dec 6, 2024

Conversation

vigji
Copy link
Contributor

@vigji vigji commented Dec 2, 2024

Addressing #1437.

Currently, all the logic in the events parser for OpenEphys data assumes that there is a single stream of events (simply positive or negative states), and it breaks down when acquiring multiple digital signals at once (states having different absolute values for different channels).

This is a simple fix that looks for rising and falling edges separately for each of the digital channels.

Add simple testing relying on existing test data, which fails before the fix and passes afterwards.

@alejoe91
Copy link
Contributor

alejoe91 commented Dec 2, 2024

Thanks @vigji! Looking great!

@zm711 zm711 changed the title Fixing ttl multichan OpenEphyBinaryRawIO: Fixing ttl multichan Dec 2, 2024
@zm711 zm711 changed the title OpenEphyBinaryRawIO: Fixing ttl multichan OpenEphysBinaryRawIO: Fixing ttl multichan Dec 2, 2024
@vigji
Copy link
Contributor Author

vigji commented Dec 2, 2024

Great, let me know if there's anything needed before merging!

Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, although I would ask for one additional comment in the code to explain what version this works for (all or v0.6+ whatever) along with what the structure of the data is.

neo/rawio/openephysbinaryrawio.py Show resolved Hide resolved
zm711
zm711 previously approved these changes Dec 3, 2024
Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for me, but Alessio knows this way better so I'll let him decide when to merge/do final review.

@zm711 zm711 added this to the 0.14.0 milestone Dec 3, 2024
Copy link
Contributor

@alejoe91 alejoe91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment

@zm711 zm711 dismissed their stale review December 6, 2024 13:06

stale

@alejoe91 alejoe91 merged commit 4061fdf into NeuralEnsemble:master Dec 6, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants